fix_: restore node config #6270
Conversation
Jenkins BuildsClick to see older builds (11)
|
qfrank
force-pushed
the
fix/node_config_migration
branch
from
77e75a4 to
8a439b4
Compare
|
You will need to rebase your branch on |
qfrank
force-pushed
the
fix/node_config_migration
branch
from
8a439b4 to
b0b61de
Compare
qfrank
force-pushed
the
fix/v1_upgrading
branch
from
beedab2 to
45d97be
Compare
qfrank
force-pushed
the
fix/node_config_migration
branch
from
b0b61de to
d7f725f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/v1_upgrading #6270 +/- ##
====================================================
- Coverage 61.79% 61.77% -0.02%
====================================================
Files 845 845
Lines 111336 111399 +63
====================================================
+ Hits 68797 68819 +22
- Misses 34572 34601 +29
- Partials 7967 7979 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
hm, not sure I an help with review |
There was a problem hiding this comment.
Since it's for a single user, and it won't be merged, I don't think we needs code review.
If it works for the user - great, if it doesn't - then there are issues in the fix 😄
I have added do not merge labels and a warning in description.
@qfrank, perhaps makes sense to convert this PR to draft as well?
--
But if the problem is solved, then a second step could be to make the PRs proper and see if we can get them merged. Probably then, the adaptions/fixes you found to work with 3esmit would help other users who never reported anything to us.
@ilmotta, do you think we need to persuade such little issues for few users?
This could even be done manually amending the DB 🤷♂️
| if currentConf.NetworkID == 0 && | ||
| currentConf.KeyStoreDir == "" && | ||
| currentConf.DataDir == "" && | ||
| currentConf.NodeKey == "" { |
There was a problem hiding this comment.
So the problem is that the node_config is empty in the database, but... why is this a problem? Can't we just use the default values, without writing them to the database?
Perhaps this is also related (and could be mitigated by) #5597. It seems that loadNodeConfig actually gives priority to DB values rather than the values passed with CreateAccount/LoginAccount/.
There was a problem hiding this comment.
So the problem is that the node_config is empty in the database, but... why is this a problem? Can't we just use the default values, without writing them to the database?
default values with backend are not enough, we don't have following values:
- verifyTransactionURL
- verifyENSURL
- verifyENSContractAddress
- verifyTransactionChainID
They're needed to pass from frontend currently.
Perhaps this is also related (and could be mitigated by) #5597.
it's not related :)
It seems that loadNodeConfig actually gives priority to DB values rather than the values passed with CreateAccount/LoginAccount
loadNodeConfig support passing inputNodeCfg which will override values from DB
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm surprised that these fields are related :) Initially I wanted to answer that my pr is related to another table in the database. But then I found this line working as a fallback:
nodeConfig.ShhextConfig.VerifyENSURL = mainnet(request.WalletSecretsConfig.StatusProxyStageName).FallbackURL
And this field will be removed soon, as well as Status Proxy will eventually be replaced by Smart Proxy. Which will always be in-memory and won't depend on the database.
also loadNodeConfig is really fragile and complex. We probably need to collect all the issues and rework them all.
Sorry for confusing, let me explain further. If old user didn't upgrade to A better solution might be to migrate configuration values like verifyXXX (e.g. verifyTransactionURL) to the backend without modifying the frontend code. However, speaking of which, even if we merge this PR, how much value does it bring, because users like 3esmit who can persist to continue using status should be a minority. cc @ilmotta |
As @qfrank mentioned above (#6270 (comment)) we believe there is or was a large pool of users who were locked out, and perhaps many of them never came back after the frustration of losing their local data. Could very much be related to our historical drop in users over time. @igor-sirotin our original intention in the mobile PR and this one in status-go is to only proceed to merge if the solution resolves the users' bug AND is of course deemed mergeable by reviewers. The user hasn't yet upgraded his prod build from App Store. We need to provide this build to him. I guess this PR for now is just to see if it sort of makes sense in everybody's head as a pragmatic solution.
The problem is how does the user modify the DB from a production build in their phone? For instance, in iOS the user was able to somehow backup the entire phone and access the Status data directory, decrypt the DBs and attempted to modify the DB in hopes of fixing the dirty DB, but to no avail yet because it's hard for the user and us to know exactly what to modify. We also can't fix directly for the user because it's their data. In Android things can be worse because if the phone is not rooted, which is the majority of cases, the user can't access the files in the data folder. And if they try to unlock the bootloader the phone data is going to be wiped out. Next week or sometime in the near future I'd love if we can start to analyze the problem of users getting locked out due Status bugs. I'll think more carefully about all this and reach out to you guys. In particular mobile users, because their situation is hopeless without a form of exporting and importing data due to the way most mobile OSes prevent data access. status-im/status-mobile#20996 (comment) In desktop OSes users already have the freedom to access and modify their data however they see fit, so it's much nicer. |
Caution
This PR is not intended to be merged
This is a continue work based on PR, the fix could fix the error:
the solution is:
however, this solution will make
loginAccountlooks not beautiful anymore.following is the opinion from @ilmotta
Relate mobile PR
Relate comment